-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix #14367: Support polyspace inline suppressions for misra c:20xx rules #8105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Should this be behind a flag? Always parsing for something very niche (and premium only?) seems undesired. |
I think that would be good. |
565b38a to
d27f25b
Compare
Since it is specific to certain MISRA rules it should only be active if those rules are active. |
da0456b to
d6d79ef
Compare
I agree it's niche use case. Especially for open source projects. imho.. if If misra checkers are not enabled then it's redundant to handle the polyspace comments. |
46e46b5 to
88ebbde
Compare
1784a61 to
46af5b1
Compare
danmar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits
| continue; | ||
| } | ||
|
|
||
| std::list<SuppressionList::Suppression> inlineSuppressions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what exact handling we have here.. but I have the feeling we might want to reuse the same handling for cppcheck/polyspace inline suppressions so isn't it a good idea to add polyspace suppressions in inlineSuppressions also? For instance the filename used for cppcheck inline suppressions why don't we want to use the same for polyspace suppressions. There is handling for "file/begin"/"end" which could be useful for polyspace also I think..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polyspace suppressions comments work quite differently from cppcheck suppression comments. So there isn't really any shared logic. But good point about the filename, I'll fix that.
| if (mFamilyMap.empty()) | ||
| return; | ||
|
|
||
| mComment = comment.substr(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My spontanous feeling about this parsing is that looks cludgy to me. mComment is assigned here in parse and tokens are grabbed from it in the nextToken(). I have the feeling I would prefer that we manipulate comment locally on the stack instead.. which makes the interface reentrant however reentrancy is not really my ultimate reason why I want this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't see the benefit of this. mComment is only modified within the call tree of parse, so there's no way to tamper with it. I think it simplifies the code to not pass it as a reference to all the other parsing functions.
| mHasPeeked = false; | ||
|
|
||
| while (true) { | ||
| const CommentKind kind = parseKind(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this handle multiple polyspace suppressions in a comment? can't there only be one suppression in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple: e7afafd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* polyspace Family_1 : Result_1_name "comment 1" polyspace Family_2 : Result_2_name "comment 2"*/
|



No description provided.